Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[문자열 덧셈 계산기] 조건희 미션 제출합니다 #2

Open
wants to merge 47 commits into
base: main
Choose a base branch
from

Conversation

whrjsgml0000
Copy link

No description provided.

@whrjsgml0000 whrjsgml0000 self-assigned this Oct 19, 2024
Copy link

@holyPigeon holyPigeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

커밋도 되게 많이 작성하시고 코드도 꼼꼼하게 짜신 것 같습니다 ㄷㄷㄷ
고생하셨습니다!!!

Comment on lines +17 to +21
- [x] 추가 구분자로 숫자를 줬을 때
- [x] 소수 넣어 보기(추가 구분자로 .을 줬을 때와 아닐 때)
- [x] 역슬래시(이스케이프 문자를 구분자로 줬을 때)
- [x] 구분자 사이에 숫자가 없을 때

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예외 케이스를 되게 다양하게 짜신 것 같습니다 👍

특히 소수 넣어 보기 같은 경우는 생각을 더 해보게 되는 것 같아요. 덧셈 계산기라는 특성상 추후 발전을 시킨다면 소수도 계산을 할 수 있어야 할텐데, 이 때 어떤 기준으로 소수를 분리할 지에 대해 고민이 되네요!

좋은 생각거리를 주셔서 감사합니다 🔥

Comment on lines +23 to +26

### 1. 커스텀 구분자가 여러 개라면?
현재 만들어진 로직은 커스텀 구분자가 한 개인 경우만을 받고 있으나 그렇지 않을 수도 있음.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1번은 진짜 생각도 못 했네요 ㄷㄷㄷ 만약 커스텀 구분자가 여러 개라면 //;\n//-\n1-2;3 이런 식으로 문자열 앞 쪽에 선언부를 여러 개 써야하지 않을까?라는 생각이 듭니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Configuration 클래스의 역할이 궁금합니다!

단순히 CalculatorServiceCalculatorController를 한꺼번에 초기화해주는 역할인지, 아니면 final 키워드를 통해 컨트롤러를 하나의 인스턴스로 관리하는 역할인지 궁금합니다.

만약 후자의 역할이라면, Configuration 클래스 자체는 새로운 인스턴스를 계속 생성할 수 있기 때문에 의도가 불투명해질 수도 있겠다는 생각이 듭니다...!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Configuration 클래스의 역할이 궁금합니다!

단순히 CalculatorServiceCalculatorController를 한꺼번에 초기화해주는 역할인지, 아니면 final 키워드를 통해 컨트롤러를 하나의 인스턴스로 관리하는 역할인지 궁금합니다.

만약 후자의 역할이라면, Configuration 클래스 자체는 새로운 인스턴스를 계속 생성할 수 있기 때문에 의도가 불투명해질 수도 있겠다는 생각이 듭니다...!

@holyPigeon 후자의 역할로 생각하고 만든 클래스긴 합니다! Configuration 을 추가적으로 만들지 못하도록 구현을 해봐야겠네요. 방금 찾아봤는데 그런걸 Singleton pattern 이라고 디자인 패턴에 있네요 하나 배워갑니당

Comment on lines 13 to 22
public void input() {
System.out.println("덧셈할 문자열을 입력해 주세요.");
String s = Console.readLine();
output(s);
}

private void output(String s) {
int sum = calculatorService.sum(s);
System.out.println("결과 : " + sum);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저 같은 경우는 입출력 기능을 따로 클래스로 만들어 분리해봤는데, 이 부분도 한 번 고려해보시면 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저 같은 경우는 입출력 기능을 따로 클래스로 만들어 분리해봤는데, 이 부분도 한 번 고려해보시면 좋을 것 같습니다!

ㄷㄷㄷㄷ 제가 그정도로 치밀하질 못했네요

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 CalculatorService에서 원본 문자열에 대한 파싱 작업을 같이 하고 계신 것으로 보입니다!

저 같은 경우에는 계산 기능과 문자열 가공 기능을 다른 클래스로 분리해봤습니다. 아무래도 우리가 실제 계산기를 사용할 때 문자열이 아닌 숫자만을 입력하는 것처럼, 계산기의 본 기능은 "계산"이고, "계산을 위해 숫자를 분리해내는 일"은 다른 클래스가 가져야 할 책임이라고 생각했기 떄문입니다!

살짝 참고해주시면 좋을 것 같습니다...!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

딱 외부에서 사용할 메서드만 public으로 선언하시고, 나머지 하위 메서드들은 private으로 선언하신 게 인상깊네요!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

계산기의 본 기능은 "계산"이고, "계산을 위해 숫자를 분리해내는 일"은 다른 클래스가 가져야 할 책임이라고 생각했기 떄문입니다!

동감합니다..!
한 클래스가 많은 책임을 가지면 확장성 및 일관성을 저해할 수 있어요.
추후 유지보수를 위해 기능을 나눠 다른 클래스로 책임을 분리하는게 좋아보입니다!
그게 객체지향이 가지는 큰 장점이라고 생각합니다😀

Comment on lines 18 to 23
/**
* 커스텀 구분자가 있는지 확인한다.
*
* @return 만약 커스텀 구분자가 있다면 true, 없다면 false 를 반환한다.
*/
private boolean checkPlusSeparator() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음엔 checkPlustSeperator 라는 메서드명을 듣고 의미를 잘 모르겠어서 조금 갸우뚱한 부분이 있었습니다!

주석을 통해 커스텀 구분자 확인에 관한 메서드라는 것을 알 수 있었네요. 사소한 부분이지만 PlusSeparator보다 CustomSeparator가 더 직관적인 이름이지 않을까 싶어요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음엔 checkPlustSeperator 라는 메서드명을 듣고 의미를 잘 모르겠어서 조금 갸우뚱한 부분이 있었습니다!

주석을 통해 커스텀 구분자 확인에 관한 메서드라는 것을 알 수 있었네요. 사소한 부분이지만 PlusSeparator보다 CustomSeparator가 더 직관적인 이름이지 않을까 싶어요!

맞아요 ㅋㅋㅋ... 처음에 만들때 메서드 이름을 저렇게 정하고 나서 바꿀 생각을 못했네욤.. 메서드랑 변수 이름 바꾸러 갑니당..

Comment on lines 24 to 28
if (s.length() >= 5 && s.startsWith("//") && s.startsWith("\\n", 3)) {
plusSeparator = String.valueOf(s.charAt(2));
s = s.substring(5);
return true;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 커스텀 구분자가 반드시 한 글자가 아닐수도 있겠다라는 생각을 했어요. 이런 가정을 덧붙이면, s.startsWith("\\n", 3) 조건의 경우 구분자가 두 글자 이상이 될 때 에러를 일으킬 수 있습니다.

이에 따라 전 s.indexOf("//") + 2, s.indexOf("\\n") 를 통해 커스텀 구분자 선언부의 시작과 끝을 설정하고서 커스텀 구분자를 추출해냈습니다.

너무 디테일하게 들어가는 걸 수도 있긴 한데 한 번 고민 정도는 해보셔도 좋을 것 같아요!

Comment on lines 53 to 61
private void hasNaN(String[] separatedStringArr) {
for (String s : separatedStringArr) {
for (char c : s.toCharArray()) {
if (c < 48 || c > 57) {
throw new IllegalArgumentException();
}
}
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2중 for문 안에 if문이 들어가서 indent depth가 3이 된 것 같습니다! 한 눈에 이해하기 조금 어려운 면이 있어서 좀 더 직관적으로 리팩토링할 방법이 있나? 찾아보시면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2중 for문 안에 if문이 들어가서 indent depth가 3이 된 것 같습니다! 한 눈에 이해하기 조금 어려운 면이 있어서 좀 더 직관적으로 리팩토링할 방법이 있나? 찾아보시면 좋을 것 같아요!

아래에 따로 메서드를 만들 생각을 했었는데 한 메서드에서 사용할 메서드를 따로 만드는게 맞나 생각했어요. 그래도 가독성을 생각하면 따로 만드는게 맞겠죠?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일단 1차적으로 대부분의 중첩 for문들이 stream문으로 바꿨을 떄 더 간결해지긴 합니다! 저라면 이렇게 바꿨을 것 같습니다.
(이게 정답은 아닐 수 있습니다 ㅋㅋㅋ)

private void hasNaN(String[] separatedStringArr) {
    if (Arrays.stream(separatedStringArr)
            .flatMapToInt(String::chars)         // 각 문자열을 문자 스트림으로 변환
            .anyMatch(c -> c < '0' || c > '9')) { // 숫자가 아닌 문자가 있는지 확인
        throw new IllegalArgumentException();
    }
}

여기서 if문의 조건절이 너무 길어지는 부분이 있어서, boolean containNonDigit(String[] separatedStringArr) 이런 느낌으로 2차적으로 메서드를 분리했을 것 같아요 😀

Comment on lines +69 to +77
private int sumSeparatedStringArr(String[] separatedStringArr) {
int sum = 0;
for (String separatedString : separatedStringArr) {
if (!separatedString.isEmpty()) {
sum += Integer.parseInt(separatedString);
}
}
return sum;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for문 대신 stream문을 사용할 수도 있습니다! 지금은 간단한 로직이라 별 차이가 안 나지만, 나중에 for문 안에 있는 로직이 복잡해지면 stream 사용을 고려해보셔도 좋을 것 같아요 😀

private int sumSeparatedStringArr(String[] separatedStringArr) {
    return Arrays.stream(separatedStringArr)
            .filter(s -> !s.isEmpty())       // 빈 문자열 필터링
            .mapToInt(Integer::parseInt)     // 문자열을 정수로 변환
            .sum();                          // 합계 계산
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for문 대신 stream문을 사용할 수도 있습니다! 지금은 간단한 로직이라 별 차이가 안 나지만, 나중에 for문 안에 있는 로직이 복잡해지면 stream 사용을 고려해보셔도 좋을 것 같아요 😀

private int sumSeparatedStringArr(String[] separatedStringArr) {
    return Arrays.stream(separatedStringArr)
            .filter(s -> !s.isEmpty())       // 빈 문자열 필터링
            .mapToInt(Integer::parseInt)     // 문자열을 정수로 변환
            .sum();                          // 합계 계산
}

가독성도 그렇고 stream 이 직관적인 면이 있어서 보기 이쁘다고 생각을 해요. 코틀린에서는 stream 문이 워낙 편해서 자주 썼었는데 자바는 좀 손에 안익더라고요... 그래도 써보려고 노력은 해봐야죠. 조언 감사합니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꼼꼼한 테스트 작성 아름답습니다...!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꼼꼼한 테스트 작성 아름답습니다...!

ㅋㅋㅋㅋㅋ 감사합니다! 제가 잡생각이 많아서 코드 짜면서 생각나는 것들 다 해보다 보니까 저렇게 됐네용

import java.util.Arrays;

public class CalculatorService {
private String s;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s라는 필드의 의미가 코드를 처음 보는 사람에게는 헷갈릴 수 있을 것 같아요.
의미상 input의 의미인 것 같은데, s라는 필드명 보다는 확실한 변수명으로 작성하는게 좋아보입니다


/**
* 덧셈 로직을 순차적으로 진행한다.
*/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드 하나하나 Java Docs를 사용하신 점이 좋은 것 같아요. 계산 관련 서비스라 로직이 복잡할 수 밖에 없는데,
설명이 친절하게 적혀있으니 이해하기 쉽네요!!

* @return 만약 커스텀 구분자가 있다면 true, 없다면 false 를 반환한다.
*/
private boolean checkCustomSeparator() {
if (s.length() >= 5 && s.startsWith("//") && s.startsWith("\\n", 3)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5, 3, 2과 같은 상수를 처리하는 부분을 enum으로 관리하면 추후 요구사항이 바뀌더라도 한 번에 처리할 수 있고, 해당 상수가 이름을 가지므로 어떤 의미를 가지는지 확실히 알 수 있기 때문에 이해하기도 더 쉬워질 것 같아요

if (Arrays.stream(separatedStringArr)
.flatMapToInt(String::chars)
.anyMatch(it -> it < '0' || it > '9')) {
throw new IllegalArgumentException();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

효과적인 에러 핸들링을 위해 예외에 메시지를 끼워넣는 것은 어떠신가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

효과적인 에러 핸들링을 위해 예외에 메시지를 끼워넣는 것은 어떠신가요?

그게 좋아보이네요! 감사합니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants